Skip to content

Fix RuntimeError of loading model with external tensor#6119

Merged
jcwchen merged 1 commit intomicrosoft:masterfrom
jcwchen:fix-external
Feb 5, 2021
Merged

Fix RuntimeError of loading model with external tensor#6119
jcwchen merged 1 commit intomicrosoft:masterfrom
jcwchen:fix-external

Conversation

@jcwchen
Copy link
Contributor

@jcwchen jcwchen commented Dec 12, 2020 •
edited
Loading

Description:

  • Set the parent directory of model path to load external tensors
  • (TODO) Resolve new error after fixing the previous one: (It will be done in another PR)
[ONNXRuntimeError] : 2 : INVALID_ARGUMENT : corrupted protobuf data: tensor shape size(1) does not match the data size(0) in prototensor shape size(1) does not match the data size(0) in proto

Motivation and Context
onnx/onnx#3157
Reload and save the onnx model with external data format will cause runtime error:

onnxruntime.capi.onnxruntime_pybind11_state.InvalidGraph: [ONNXRuntimeError] : 10 : INVALID_GRAPH : Load model from chitchatexporttest/median_sampled_qgen_48_past_fp32/median_sampled_qgen_48_past_fp32.onnx failed:This is an invalid model. Error in Node:ConstantOfShape_11958 : Data of TensorProto ( tensor name: ConstantOfShape_11958_attr::value) should be stored in median_sampled_qgen_48_past_fp32.onnx.data, but it doesn't exist or is not accessible.

@jcwchen jcwchen requested a review from a team as a code owner December 12, 2020 00:05
guoyu-wang
guoyu-wang reviewed Dec 12, 2020
View reviewed changes
yuslepukhin
yuslepukhin reviewed Dec 14, 2020
View reviewed changes
@jcwchen jcwchen changed the title [WIP] Fix RuntimeError of reloading model with external tensor Fix RuntimeError of reloading model with external tensor Dec 17, 2020
@jcwchen jcwchen changed the title Fix RuntimeError of reloading model with external tensor Fix RuntimeError of loading model with external tensor Dec 18, 2020
@jcwchen
Copy link
Contributor Author

jcwchen commented Dec 18, 2020

All updated. Thank you for the review and this PR is ready.

askhade
askhade previously approved these changes Jan 8, 2021
View reviewed changes
guoyu-wang
guoyu-wang previously approved these changes Jan 12, 2021
View reviewed changes
snnn
snnn reviewed Jan 12, 2021
View reviewed changes
onnxruntime/core/graph/graph.cc Outdated
Copy link
Contributor

@snnn snnn Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it must be ended by '/'?

Copy link
Contributor Author

@jcwchen jcwchen Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/onnx/onnx/blob/bd2ff65b775580d21ab38cb655a04854c348ff49/onnx/checker.cc#L133
It is required here because the code in ONNX does not handle path join...

Copy link
Contributor

@snnn snnn Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then onnx should fix it! It is always safe to add an additional path separator there when you do path join. But, it seems a little bit wired to do it here without knowing the result will only be used in path join.

Copy link
Contributor

@snnn snnn Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And instead of calling std::ifstream, it should use fstat to test if a file exists: https://linux.die.net/man/2/fstat

Copy link
Contributor Author

@jcwchen jcwchen Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I am proposing a PR in ONNX to fix it before checking this PR in. Thank you for bringing this up.

Copy link
Contributor Author

@jcwchen jcwchen Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the PR to fix that in ONNX: onnx/onnx#3214

@jcwchen jcwchen mentioned this pull request Jan 14, 2021
Merged
@jcwchen jcwchen dismissed stale reviews from guoyu-wang and askhade via 062b89b January 29, 2021 16:46
@jcwchen
Copy link
Contributor Author

jcwchen commented Jan 30, 2021

A path join fix has been merged in ONNX: onnx/onnx#3214. Then this PR would be easier. Set the model directory without giving the separator and update ONNX commit. This PR is ready for review. Thanks.

@jcwchen jcwchen merged commit f2ce3aa into microsoft:master Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yuslepukhin yuslepukhin yuslepukhin left review comments

@askhade askhade askhade left review comments

+2 more reviewers

@snnn snnn snnn approved these changes

@guoyu-wang guoyu-wang guoyu-wang left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jcwchen @snnn @askhade @yuslepukhin @guoyu-wang